Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make SelectedDeclarationProvider argument-aware #5740

Merged
merged 10 commits into from
Apr 24, 2021
Merged

Make SelectedDeclarationProvider argument-aware #5740

merged 10 commits into from
Apr 24, 2021

Conversation

retailcoder
Copy link
Member

This pull requests makes the SelectionService resolve the selected ParameterDeclaration given a selection that's inside an ArgumentContext; the parameter declaration is used for the "current selection" caption when the argument is an expression that doesn't otherwise resolve to another declaration, like when an argument is a literal:

literal string argument to MsgBox function shows it's the Prompt parameter and it's a Variant

Or when the argument is an expression with late-bound elements selected:

Sheet1.Cells arguments are late-bound per the implicit default member call

But any early-bound more specific sub-expression that resolves to a declaration remains the caption for other parameters:

image

Named arguments in a different order than defined, doesn't break:

image

Missing arguments aren't handled and behave as previously:

image

@retailcoder
Copy link
Member Author

retailcoder commented Apr 21, 2021 •
edited

Tweaking "find all references" to locate argument refs (not just the named ones) for parameter declarations would be a nice next step.

@bclothier
Copy link
Contributor

Sweet enhancement!

2 small comments based on the images:

  1. You probably may want to have it display Object, rather than IDispatch (2nd image) as that is more familiar to the users.

  2. I am not sure I agree that missing argument should be skipped. I don't know if it's easy/feasible but I think I would rather see the missing argument name & type shown as a hint. Think about how many time people made a mistake of MsgBox "foo", "my title"....

@retailcoder
Copy link
Member Author

@bclothier 1 is a good idea, I'll do that. Hadn't realized 2 is possible when there are no named arguments, this would work too.

@retailcoder
Copy link
Member Author

Missing arguments work!

missing argument info

The argument context excludes the preceding whitespace, so I made a little dance and I'm treating the preceding whitespace token as part of the argument - this has the side-effect of lining up the "selected/current argument" with the VBE's parameter quick-info:

resolving to Buttons parameter despite being in the middle of whitespace and line continuations

quick-info showing the Buttons parameter in bold

@retailcoder
Copy link
Member Author

IDispatch is now shown as 'Object' in the commandbar

@retailcoder
Copy link
Member Author

Okay now this is getting pretty nice..

image

bclothier
bclothier approved these changes Apr 23, 2021
View reviewed changes
Copy link
Contributor

@bclothier bclothier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice work! Only one little nitpick about magical literals.

Rubberduck.Core/UI/Command/MenuItems/CommandBars/IContextFormatter.cs Outdated Show resolved Hide resolved
@retailcoder retailcoder merged commit b9199b0 into rubberduck-vba:next Apr 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BZngr BZngr BZngr left review comments

@bclothier bclothier bclothier approved these changes

@MDoerner MDoerner Awaiting requested review from MDoerner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@retailcoder @bclothier @BZngr